fix(ai): preserve Error message, stack, and cause chain on $ai_error#3584
fix(ai): preserve Error message, stack, and cause chain on $ai_error#3584Ashut0sh-mishra wants to merge 2 commits into
Conversation
JSON.stringify on an Error instance returns {} because
ame,
message, stack, and cause are non-enumerable properties. As a result,
$ai_generation events captured via withTracing were storing a stripped
down blob on $ai_error (and on the deeper cause levels), making it
nearly impossible to debug what actually went wrong when an SDK like
@ai-sdk/gateway threw a chained error.
Add a serializeError helper that explicitly normalizes Error instances to
plain objects (name / message / stack / cause + own enumerable properties
like statusCode,
esponse, �alidationError), recursing into the cause
chain and into nested plain objects. A WeakSet of seen references guards
against circular cause graphs.
captureAiGeneration now passes the error through serializeError before
JSON.stringify, so $ai_error keeps the same string shape (backwards
compatible with consumers that expect a string) but carries the real data.
Closes PostHog#3556
Co-authored-by: nik464 <nik464@users.noreply.github.com>
|
@Ashut0sh-mishra is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/ai/src/utils.ts:605-638
**`seen` marks shared (non-circular) references as `[Circular]`**
The `WeakSet` is shared across the entire object-graph traversal, not just the current call-stack path. This means any object that appears at two distinct locations in the error structure — a "diamond" reference (e.g. the same `response` object attached both to `err.response` and `err.cause.response`) — will serialize correctly the first time and then produce `'[Circular]'` the second time, even though there is no actual cycle. A path-tracking approach (passing the current ancestor set only, or cloning the set at each branch point) would give accurate output, though it comes at a higher allocation cost. For the primary cause-chain use case the current approach is fine, but it's worth documenting this limitation in the JSDoc since the function is exported.
Reviews (1): Last reviewed commit: "fix(ai): preserve Error message, stack, ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses greptile review on PostHog#3584: the WeakSet is shared across the whole traversal, so a diamond (non-circular) reference will be reported as `[Circular]` after its first visit. Documented this as intentional (keeps allocation low; correct for the primary linear cause-chain use case) so future callers aren't surprised. Co-authored-by: nik464 <nik464@users.noreply.github.com>
|
Thanks @greptile-apps! Good point on the diamond-reference case. Documented the limitation in the JSDoc as intentional (keeps allocation O(visited); the linear cause-chain is the primary use case). Pushed. |
1 similar comment
|
Thanks @greptile-apps! Good point on the diamond-reference case. Documented the limitation in the JSDoc as intentional (keeps allocation O(visited); the linear cause-chain is the primary use case). Pushed. |
|
The JSDoc update is clear and thorough — the note about |
|
The JSDoc reads clearly — the trade-off is well-stated and the "primary use case (linear cause chains)" framing sets the right expectation. The implementation looks solid: WeakSet allocation is indeed O(visited) and the behavior is deterministic and predictable for the actual production case (SDK error chains). LGTM on this approach. 👍 |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/ai/src/utils.ts:633-637
The `key in obj` check walks up the prototype chain of `obj`, so any enumerable own property on the Error whose key coincidentally matches a property on `Object.prototype` (e.g. `toString`, `valueOf`, `constructor`) would be silently skipped. `Object.hasOwn` limits the check to own properties of `obj`, which is what's actually intended here.
```suggestion
for (const key of Object.keys(err)) {
if (!Object.hasOwn(obj, key)) {
obj[key] = serializeError((err as Record<string, unknown>)[key], seen)
}
}
```
Reviews (2): Last reviewed commit: "docs(ai): document shared-seen-set limit..." | Re-trigger Greptile |
|
same as #3605 i think |
| // `$ai_error` show up as an empty-ish blob on the dashboard. Normalize | ||
| // Errors to plain objects first so the fields survive serialization. | ||
| // See https://github.com/PostHog/posthog-js/issues/3556 | ||
| $ai_error: sanitizeValues(JSON.stringify(serializeError(options.error))), |
There was a problem hiding this comment.
Just dropping a comment in case it's useful, inside posthog core we have an error parser that creates an exception list from any object which is the format we use at error tracking. This will also pull the causes from the error to reconstruct the exception chain. You could potentially reuse some of our component as well to display the stacktrace on the UI if the format is the same.
|
Ended up merging #3605 in response to another ticket, but the implementation was pretty similar. @Ashut0sh-mishra, thanks for your contribution! You should be able to get the new behaviour by updating to @hpouillot, I wasn't aware of that error helper. Will probably switch the implementation to it for consistency at a later point. |
4 participants
Problem
JSON.stringifyon a JavaScriptErrorinstance returns{}becausename,message,stack, andcauseare non-enumerable. ThecaptureAiGenerationpath storedJSON.stringify(options.error)directlyon ``, so events captured via
withTracingended up with astripped-down blob — no message, no stack, no usable cause chain — making
`GatewayResponseError` and friends nearly impossible to debug from the
dashboard.
Closes #3556
Fix
serializeErrorhelper inpackages/ai/src/utils.tsthatnormalizes
Errorinstances to plain objects (name/message/stack/causeplus own enumerable properties such asstatusCode,response,validationError), recurses through the cause chain andthrough nested plain objects, and guards against circular cause graphs
with a
WeakSet.captureAiGenerationnow passes the error throughserializeErrorbefore
JSON.stringify, so `` keeps the same string shape(backwards compatible with anything that already reads it as JSON text)
but carries the actual error data.
Tests
Added to
packages/ai/tests/captureAiGeneration.test.ts:Erroris captured and the test assertsmessage,stack,statusCode, and the recursivecauselevelsare all present on the parsed `` JSON.
is replaced with
'[Circular]'rather than overflowing the stack.The existing
error pathparametric tests are untouched and stillassert `` is a string, so this is backwards compatible.
Reproducer (from the issue)
The original reproducer aborts mid-stream against
@ai-sdk/gateway;after this change the dashboard's `` payload contains the
GatewayResponseError`message`, `statusCode`, and the full`validationError` -> `ZodError` cause chain instead of a blob like
`{ statusCode: 500, cause: {}, name: 'GatewayResponseError' }`.